Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add Bound constructors for PyByteArray and PyMemoryView #3786

Merged
merged 2 commits into from
Jan 31, 2024
Merged

add Bound constructors for PyByteArray and PyMemoryView #3786

merged 2 commits into from
Jan 31, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Jan 30, 2024

Part of #3684

The adds the Bound constructors for PyByteArray and PyMemoryView.

Since the original from constructor used as_ptr(), which returns a borrowed ptr, I used a borrowed &Bound for the new one. Please correct me if I'm wrong here.

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jan 30, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3786 will improve performances by 12.92%

Comparing Icxolu:bytearray (b14dbcf) with main (6040d93)

Summary

⚡ 1 improvements
✅ 78 untouched benchmarks

Benchmarks breakdown

Benchmark main Icxolu:bytearray Change
⚡ not_a_list_via_downcast 242.8 ns 215 ns +12.92%

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 30, 2024
davidhewitt
davidhewitt reviewed Jan 30, 2024
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant, thank you yet again! Good pick to choose these ones; I'd totally forgotten they needed constructors added still.

Since the original from constructor used as_ptr(), which returns a borrowed ptr, I used a borrowed &Bound for the new one. Please correct me if I'm wrong here.

&Bound is exactly right based on what we've been doing elsewhere 👍

Just one tiny detail which is a suggestion to improve the surrounding code a tiny bit while we're there...

src/types/bytearray.rs Outdated Show resolved Hide resolved
davidhewitt
davidhewitt approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks again!

@davidhewitt davidhewitt added this pull request to the merge queue Jan 31, 2024
@davidhewitt
Copy link
Member

Btw, if you are interested in some reviewing as well as coding, I've started keeping a list on #3684, some help getting those reviewed and merged would go a long way towards getting this new API shipped!

Merged via the queue into PyO3:main with commit aa1a986 Jan 31, 2024
37 of 38 checks passed
@Icxolu
Copy link
Contributor Author

Icxolu commented Jan 31, 2024

Btw, if you are interested in some reviewing as well as coding, I've started keeping a list on #3684, some help getting those reviewed and merged would go a long way towards getting this new API shipped!

I'll have a look and leave some comments.

@Icxolu Icxolu deleted the bytearray branch January 31, 2024 17:25
@Icxolu Icxolu mentioned this pull request Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Icxolu @davidhewitt